Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

revert!: rsmt2d#277 and rsmt2d#287 #295

Merged
merged 2 commits into from
Feb 7, 2024
Merged

revert!: rsmt2d#277 and rsmt2d#287 #295

merged 2 commits into from
Feb 7, 2024

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Feb 3, 2024 •
edited
Loading

While attempting to bump celestia-app to the v0.12.0-rc2, I noticed that the RegisterTree design leaks an implementation detail to celestia-app: the registering and managing of treeNames. Celestia-app has two categories of of trees:

  1. erasured namespaced merkle tree in nmt_wrapper.go
  2. EDS subtree root cacher nmt_caching.go

Each of those categories has trees based on square size and NMT options. Celestia-app needs to be careful to register all the appropriate trees once (and only once) before they are used (via Compute or Import). I'd like to explore a less breaking option to get celestia-node the original desired feature which was #275. In the meantime, I think we should revert the two big breaking changes so that main can remain release-able.

Revert #277
Revert #287
Closes #295 because no longer relevant if we merge this.

@rootulp rootulp self-assigned this Feb 3, 2024
@codecov Codecov
Copy link

codecov bot commented Feb 3, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (d6c118c) 80.89% compared to head (eaac726) 82.24%.
Report is 3 commits behind head on main.

Files Patch % Lines
extendeddatasquare.go 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #295      +/-   ##
==========================================
+ Coverage   80.89%   82.24%   +1.34%     
==========================================
  Files           8        7       -1     
  Lines         869      614     -255     
==========================================
- Hits          703      505     -198     
+ Misses        119       66      -53     
+ Partials       47       43       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rootulp rootulp marked this pull request as ready for review February 3, 2024 17:41
@rootulp rootulp mentioned this pull request Feb 3, 2024
Closed
@staheri14
Copy link
Contributor

Mind providing a link to nmt_caching.go? Thanks

@rootulp
Copy link
Collaborator Author

rootulp commented Feb 5, 2024 •
edited
Loading

Sure! I updated the original post.

@staheri14
Copy link
Contributor

staheri14 commented Feb 7, 2024 •
edited
Loading

Has nmt_caching.go been utilized in the rsmt2d context? It seems not to conform to the Tree interface, prompting questions about its necessity for registration. Any clarification on this would be greatly appreciated!
Or it is a future feature that were to be implemented (i.e., making the nmt_caching implement the Tree interface)?

@rootulp
Copy link
Collaborator Author

rootulp commented Feb 7, 2024

I think nmt_caching.go is used in rsmt2d context because this test. Also this line returns a tree that conforms to the rsmt2d Tree interface.

@staheri14
Copy link
Contributor

Thank you for sharing the links. You're correct; the NewSubtreeCacher.Constructor is indeed used in the test, it generates an ErasuredNamespacedMerkleTree with caching enabled. It seems, though, that differentiation might be necessary when registering tree names. However, given that the underlying tree remains consistent, do you think there's potential to simplify by consolidating the tree names into a single one? Just pondering this possibility.

Additionally, I'm eager to hear your perspective on the challenges associated with "Celestia-app needs to be careful to register all the appropriate trees once (and only once) before they are used ". What do you see as the main concerns with this requirement?

Nevertheless, I am going to approve and leave the final decision up to you.

@rootulp rootulp merged commit aab15aa into celestiaorg:main Feb 7, 2024
5 checks passed
@rootulp rootulp deleted the rp/revert-register-tree branch February 7, 2024 20:41
0xchainlover pushed a commit to celestia-org/rsmt2d that referenced this pull request Aug 1, 2024
@rootulp
revert!: rsmt2d#277 and rsmt2d#287 (#295)
51f8909 
While attempting to bump celestia-app to the v0.12.0-rc2, I noticed that
the `RegisterTree` design leaks an implementation detail to
celestia-app: the registering and managing of `treeName`s. Celestia-app
has two categories of of trees:
1. erasured namespaced merkle tree in
[nmt_wrapper.go](https://github.com/celestiaorg/celestia-app/blob/main/pkg/wrapper/nmt_wrapper.go)
2. EDS subtree root cacher
[nmt_caching.go](https://github.com/celestiaorg/celestia-app/blob/main/pkg/inclusion/nmt_caching.go)

Each of those categories has trees based on square size and NMT options.
Celestia-app needs to be careful to register all the appropriate trees
once (and only once) before they are used (via `Compute` or `Import`).
I'd like to explore a less breaking option to get celestia-node the
original desired feature which was
celestiaorg/rsmt2d#275. In the meantime, I
think we should revert the two big breaking changes so that main can
remain release-able.

Revert celestiaorg/rsmt2d#277
Revert celestiaorg/rsmt2d#287
Closes celestiaorg/rsmt2d#295 because no longer
relevant if we merge this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-forbes evan-forbes evan-forbes approved these changes

@staheri14 staheri14 staheri14 approved these changes

@Wondertan Wondertan Awaiting requested review from Wondertan

Assignees

@rootulp rootulp

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rootulp @staheri14 @evan-forbes